[ISSUE #10995] Stabilize client offline receipt handle test - #10996
[ISSUE #10995] Stabilize client offline receipt handle test#10996unbridled-41 wants to merge 1 commit into
Conversation
Signed-off-by: unbridled-41 <[email protected]>
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Test-only fix that stabilizes DefaultReceiptHandleManagerTest#testClientOffline by properly stubbing the async changeInvisibleTime() cleanup path and using Awaitility for the async assertion. Clean, minimal, and well-documented.
Correctness — The root cause is correctly identified: unstubbed changeInvisibleTime() returns null from Mockito, causing NPE in the async callback's whenComplete(). Stubbing with CompletableFuture.completedFuture(new AckResult()) ensures the cleanup path completes without error.
Tests — Awaitility usage is appropriate here. The 1-second timeout is reasonable since the stubbed future is already completed, so the wait should resolve nearly instantly. The Mockito.verify() inside untilAsserted correctly validates both the invocation and the final state.
Compatibility — No production code changes. Test-only scope matches the issue description exactly.
LGTM — no issues found.
Automated review by github-manager-bot
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #10996 +/- ##
=============================================
- Coverage 48.59% 48.49% -0.11%
+ Complexity 13680 13643 -37
=============================================
Files 1381 1381
Lines 101475 101475
Branches 13190 13190
=============================================
- Hits 49313 49206 -107
- Misses 46163 46248 +85
- Partials 5999 6021 +22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Test stabilization for testClientOffline — properly mocks the async changeInvisibleTime call and uses awaitility for async assertions instead of immediate checks.
Observations
- ✅ Correctly mocks the async operation to return a completed future
- ✅ Uses
await().atMost()for async verification, which is more robust than immediate assertions - ✅ Fixes missing newline at end of file
- The change makes the test more reliable and less prone to flakiness
Suggestions
- Consider adding a brief comment in the test explaining what race condition or timing issue this fixes, for future maintainers
LGTM — good test stabilization.
Automated review by github-manager-bot
5 participants
Which Issue(s) This PR Fixes
Brief Description
DefaultReceiptHandleManagerTest#testClientOfflinetriggers asynchronous receipt-handle cleanup afterCLIENT_UNREGISTER.The test previously did not stub
messagingProcessor.changeInvisibleTime()for the cleanup path. The unstubbed Mockito invocation returnsnull, and the asynchronousReturnHandleGroupWorkerThreadcan then throw aNullPointerExceptionwhen the test listener callswhenComplete().When this happens, the receipt handle is not removed and
returnHandleGroup()may insert the non-empty group back intoreceiptHandleGroupMap. Because the test immediately asserted that the map was empty after triggeringCLIENT_UNREGISTER, the assertion was timing-sensitive and could fail in Coverage CI.This change is test-only:
changeInvisibleTime()with a completedCompletableFuturefor the cleanup path;How Did You Test This Change?
Tested with JDK 8:
DefaultReceiptHandleManagerTest#testClientOffline: 50/50 repeated runs passedDefaultReceiptHandleManagerTest: 12/12 tests passedBUILD SUCCESSgit diff --check: passed